Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[icloud] Fix NPE in AccountBridgeHandler #7087

Merged
merged 2 commits into from Mar 1, 2020
Merged

[icloud] Fix NPE in AccountBridgeHandler #7087

merged 2 commits into from Mar 1, 2020

Conversation

radokristof
Copy link
Contributor

@radokristof radokristof commented Mar 1, 2020 •
edited

Hope this makes sense.
I got this NPE few days ago:

2020-02-29 22:10:53.325 [WARN ] [mmon.WrappedScheduledExecutorService] - Scheduled runnable ended with an exception: 
java.lang.NullPointerException: null
	at org.openhab.binding.icloud.internal.handler.ICloudAccountBridgeHandler.refreshData(ICloudAccountBridgeHandler.java:170) ~[?:?]
	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511) ~[?:1.8.0_222]
	at java.util.concurrent.FutureTask.runAndReset(FutureTask.java:308) ~[?:1.8.0_222]
	at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.access$301(ScheduledThreadPoolExecutor.java:180) ~[?:1.8.0_222]
	at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:294) ~[?:1.8.0_222]
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149) [?:1.8.0_222]
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624) [?:1.8.0_222]
	at java.lang.Thread.run(Thread.java:748) [?:1.8.0_222]

So I think only iCloudData can be null here.
This and every non-catch Exceptions interrupts the refreshData thread, meaning that the binding won't refresh the data automatically.
Or should we somehow restart the threads again when something causes it to interrupt?

@radokristof
Fix NPE in AccountBridgeHandler
185fb0a 
Signed-off-by: Kristof Rado <rado.krisi@gmail.com>
@TravisBuddy
Copy link

Travis tests were successful

Hey @radokristof,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

@J-N-K
Copy link
Member

J-N-K commented Mar 1, 2020

Makes sense. But it also means that the methos signature

openhab-addons/bundles/org.openhab.binding.icloud/src/main/java/org/openhab/binding/icloud/internal/ICloudDeviceInformationParser.java

Line 32 in 4ad6329

public ICloudAccountDataResponse parse(String json) throws JsonSyntaxException {
needs to be changed to

public @Nullable ICloudAccountDataResponse parse(String json) throws JsonSyntaxException

This might result in some more errors where the (wrong) non-null assumption is used. Please have an eye on this.

@TravisBuddy
Copy link

Travis tests were successful

Hey @radokristof,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

@radokristof
Copy link
Contributor Author

radokristof commented Mar 1, 2020 •
edited

@J-N-K yes you are right. Previously when I worked on the binding, I assumed that JsonSyntaxException is thrown every time when the JSON is incorrect or null.
But now looking at the docs:

an object of type T from the string. Returns null if json is null.

However the json is checked against null before, so it can't be null there.
I'm a little bit confused now...

@J-N-K
Copy link
Member

J-N-K commented Mar 1, 2020

Seems to be this issue: google/gson#540. See also https://stackoverflow.com/questions/42396634/prevent-gson-from-returning-null-when-deserializing-empty-strings.

IMO the documentation for fromJson is wrong.

@J-N-K
Copy link
Member

J-N-K commented Mar 1, 2020

So the null-check could be omitted and the method

public @Nullable ICloudAccountDataResponse parse(@Nullable String json) throws JsonSyntaxException

@radokristof
Added Nullable annotation
c8a5f30 
Signed-off-by: Kristof Rado <rado.krisi@gmail.com>
@radokristof
Copy link
Contributor Author

@J-N-K Thanks! Already pushed a commit that contains that fix.

Thanks for your help!

@TravisBuddy
Copy link

Travis tests were successful

Hey @radokristof,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

@J-N-K J-N-K added the bug An unexpected problem or unintended behavior of an add-on label Mar 1, 2020
@J-N-K J-N-K merged commit dc63980 into openhab:2.5.x Mar 1, 2020
@radokristof radokristof deleted the icloud_patch branch March 1, 2020 22:14
@kaikreuzer kaikreuzer added this to the 2.5.3 milestone Mar 18, 2020
leluna pushed a commit to leluna/openhab2-addons that referenced this pull request Mar 21, 2020
@radokristof @leluna
[icloud] Fix NPE in AccountBridgeHandler (openhab#7087)
38aaa40 
* Fix NPE in AccountBridgeHandler

Signed-off-by: Kristof Rado <rado.krisi@gmail.com>
Signed-off-by: leluna <hengrui.jiang@googlemail.com>
Hans-Reiner pushed a commit to Hans-Reiner/openhab2-addons that referenced this pull request Apr 11, 2020
@radokristof @Hans-Reiner
[icloud] Fix NPE in AccountBridgeHandler (openhab#7087)
038b965 
* Fix NPE in AccountBridgeHandler

Signed-off-by: Kristof Rado <rado.krisi@gmail.com>
Signed-off-by: Hans-Reiner Hoffmann <hans-reiner.hoffmann@gmx.de>
markus7017 pushed a commit to markus7017/openhab-addons that referenced this pull request May 29, 2020
@radokristof @markus7017
[icloud] Fix NPE in AccountBridgeHandler (openhab#7087)
bd335a3 
* Fix NPE in AccountBridgeHandler

Signed-off-by: Kristof Rado <rado.krisi@gmail.com>
LoungeFlyZ pushed a commit to LoungeFlyZ/openhab2-addons that referenced this pull request Jun 8, 2020
@radokristof @LoungeFlyZ
[icloud] Fix NPE in AccountBridgeHandler (openhab#7087)
099d726 
* Fix NPE in AccountBridgeHandler

Signed-off-by: Kristof Rado <rado.krisi@gmail.com>
J-N-K pushed a commit to J-N-K/openhab-addons that referenced this pull request Jul 14, 2020
@radokristof @J-N-K
[icloud] Fix NPE in AccountBridgeHandler (openhab#7087)
8c8841f 
* Fix NPE in AccountBridgeHandler

Signed-off-by: Kristof Rado <rado.krisi@gmail.com>
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
@radokristof @andrewfg
[icloud] Fix NPE in AccountBridgeHandler (openhab#7087)
f05eba7 
* Fix NPE in AccountBridgeHandler

Signed-off-by: Kristof Rado <rado.krisi@gmail.com>
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
@radokristof @andrewfg
[icloud] Fix NPE in AccountBridgeHandler (openhab#7087)
dcae4e3 
* Fix NPE in AccountBridgeHandler

Signed-off-by: Kristof Rado <rado.krisi@gmail.com>
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
@radokristof @andrewfg
[icloud] Fix NPE in AccountBridgeHandler (openhab#7087)
214bf91 
* Fix NPE in AccountBridgeHandler

Signed-off-by: Kristof Rado <rado.krisi@gmail.com>
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
@radokristof @andrewfg
[icloud] Fix NPE in AccountBridgeHandler (openhab#7087)
2ab1120 
* Fix NPE in AccountBridgeHandler

Signed-off-by: Kristof Rado <rado.krisi@gmail.com>
DaanMeijer pushed a commit to DaanMeijer/openhab-addons that referenced this pull request Sep 1, 2020
@radokristof @DaanMeijer
[icloud] Fix NPE in AccountBridgeHandler (openhab#7087)
39873ff 
* Fix NPE in AccountBridgeHandler

Signed-off-by: Kristof Rado <rado.krisi@gmail.com>
Signed-off-by: Daan Meijer <daan@studioseptember.nl>
markus7017 pushed a commit to markus7017/openhab-addons that referenced this pull request Sep 19, 2020
@radokristof @markus7017
[icloud] Fix NPE in AccountBridgeHandler (openhab#7087)
9594520 
* Fix NPE in AccountBridgeHandler

Signed-off-by: Kristof Rado <rado.krisi@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@J-N-K J-N-K J-N-K approved these changes

Assignees
No one assigned
Labels
bug An unexpected problem or unintended behavior of an add-on
Projects
None yet
Milestone
2.5.3
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@radokristof @TravisBuddy @J-N-K @kaikreuzer